Skip to content

Conversation

@petk
Copy link
Member

@petk petk commented Jun 28, 2019

These symbols are defined by the build system to check the presence of the standard C89 headers. Since C89 is really widely adopted across systems already, these checks are obsolete today and not needed.

http://port70.net/~nsz/c/c89/c89-draft.html#4.1.2

Some of these checks are also removed since PHP-7.4.

These symbols are defined by the build system to check the presence of
the standard C89 headers. Since C89 is really widely adopted across
systems already, these checks are obsolete today and not needed.

http://port70.net/~nsz/c/c89/c89-draft.html#4.1.2

Some of these checks are also removed since PHP-7.4.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.652% when pulling ce349f0 on petk:patch-c89 into 60c159a on php:master.

@petk
Copy link
Member Author

petk commented Jul 19, 2019

Is here even anyone checking pull requests?

@php-pulls php-pulls closed this in feeb28a Jul 20, 2019
@petk petk deleted the patch-c89 branch July 20, 2019 22:34
@Jan-E
Copy link
Contributor

Jan-E commented Jul 20, 2019

Is here even anyone checking pull requests?

@petk Who did you tickle to merge the PR's? There are some more PR's waiting to be reviewed and merged...

@petk
Copy link
Member Author

petk commented Jul 20, 2019

I've merged it since no one is looking these I think and I know very well the changes in this PR to be ok... But issue is that I'm not that "at home" with other C parts and changes in pull requests. I would much prefer for someone who is also releasing this extension on pecl to look at pull requests.

Is there something that is confirmed ok and can be merged here?

@Jan-E
Copy link
Contributor

Jan-E commented Jul 20, 2019

#14 (comment)
I tested the PHP 7.4 patch locally with a running SOLR server. With the possible exception of 1 32/64 bits error all tests passed.

@Jan-E
Copy link
Contributor

Jan-E commented Jul 20, 2019

#15 is confirmed as well.

@Jan-E
Copy link
Contributor

Jan-E commented Jul 20, 2019

Looks like this one is OK as well:
#16 (comment)

@petk
Copy link
Member Author

petk commented Jul 21, 2019

Awesome then. Merges coming up. Thank you for the reviews...

@Jan-E
Copy link
Contributor

Jan-E commented Jul 21, 2019

@0mars said he would look at them this weekend:
#16 (comment)

@Jan-E
Copy link
Contributor

Jan-E commented Jul 21, 2019

#14 (comment)
I tested the PHP 7.4 patch locally with a running SOLR server. With the possible exception of 1 32/64 bits error all tests passed.

For @0mars and @petk Only the first commit of https://github.com/php/pecl-search_engine-solr/pull/14/commits should be merged. The ones relating to appVeyor can be ignored.

@petk
Copy link
Member Author

petk commented Jul 21, 2019

@Jan-E about #16 I'm not sure I'm up to this merge. I'm not sure I'll even be able to test this properly - there are changes of the bug title.
#14 can you please split the appveyor change into a separate pull request then...

Thanks. I still think it is best for someone who has been around this extension longer than I to look at these.

@Jan-E
Copy link
Contributor

Jan-E commented Jul 21, 2019

The appveyor changes were just to show some test results online, but many were skipped because of the lack of a running SOLR server on Appveyor. See this ancient comment of @0mars about that one:
5039719#commitcomment-24172849

I removed the Appveyor changes from the PR: #14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants